Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add multipart writer #27

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Add multipart writer #27

wants to merge 9 commits into from

Conversation

eXenon
Copy link
Member

@eXenon eXenon commented Apr 26, 2019

Major update to the package including :

  • Moving the existing code to a separate Reader module
  • Adding a Writer module able to write multipart request bodies (compatible with cohttp-lwt)
  • Re-organizing unittests in order to include the tests of the writer module as well as some combined writer + reader tests to ensure compatiblity
  • Fixing several errors in the reader code where it would append newlines at the end of files

eXenon added 2 commits April 26, 2019 11:18
@eXenon
Add multipart writer
2ae1169 
Move the reader into a separate module
@eXenon
Major refactoring of tests
d424217
@eXenon eXenon requested a review from bbc2 April 26, 2019 09:24
@eXenon eXenon force-pushed the add-multipart-writer branch from 8a1fb5c to b43cc8b Compare June 3, 2019 13:18
@eXenon
Copy link
Member Author

eXenon commented Jun 3, 2019

Implemented improved API and added unittests.
The unittests are a little clunky, but I don't think we can do much better given the API that we chose.

@eXenon eXenon force-pushed the add-multipart-writer branch from b43cc8b to add1f65 Compare June 3, 2019 13:26
bbc2
bbc2 requested changes Jun 4, 2019
View reviewed changes
Copy link
Member

@bbc2 bbc2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new API is better although I think it has issues that I hadn't thought of (see related comment). Other comments are mostly trivial. Also, it would be good to have this pass the CI.

test/test_reader.ml Outdated Show resolved Hide resolved
test/test_reader.ml Outdated Show resolved Hide resolved
test/test_reader.ml Show resolved Hide resolved
test/test_reader.ml Outdated Show resolved Hide resolved
test/test_reader.ml Outdated Show resolved Hide resolved
lib/multipart_form_data.mli Outdated Show resolved Hide resolved
lib/writer.ml Outdated Show resolved Hide resolved
lib/multipart_form_data.mli Outdated Show resolved Hide resolved
lib/reader.ml Outdated Show resolved Hide resolved
test/utils.ml Outdated Show resolved Hide resolved
@bbc2
Copy link
Member

bbc2 commented Jun 4, 2019

The unittests are a little clunky, but I don't think we can do much better given the API that we chose.

I think the way you handled it (i.e. by converting incomparable values to comparable ones) is good.

@eXenon eXenon force-pushed the add-multipart-writer branch 2 times, most recently from fdc803f to 2eb4b74 Compare June 7, 2019 11:44
@eXenon
New, simpler API
495f21e 
Fix bug where the all the files would end up in the same stream
Add test with multiple «files»
Add test/utils.mli
Removed old files and unused functions
@eXenon eXenon force-pushed the add-multipart-writer branch from 2eb4b74 to 495f21e Compare June 7, 2019 11:54
eXenon added 3 commits June 7, 2019 15:00
@eXenon
New README
90cf740
@eXenon
Made boundary optional
cc9260a 
Rename requests -> parts in write function
@eXenon
Add mli files
5255833 
And remove a lot of dead code
@eXenon eXenon force-pushed the add-multipart-writer branch from 5d0f1a3 to a98dcab Compare June 11, 2019 07:18
@eXenon eXenon force-pushed the add-multipart-writer branch from a98dcab to 0c2383e Compare June 11, 2019 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbc2 bbc2 bbc2 requested changes

Assignees

@bbc2 bbc2

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eXenon @bbc2